[Otlp] Add disk retry enablement #5527

vishweshbankwar · 2024-04-09T23:49:25Z

Towards #4791
Design discussion issue #

Changes

Adds the ability to opt-in for retry at a later time by storing telemetry offline on disk when exporter experiences transient network failures.

The persistent storage retry handler was added in #5460. This change is wiring up the exporters for using the handler.

TODO: Additional documentation to address #5527 (comment)

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

codecov · 2024-04-09T23:57:22Z

Codecov Report

Attention: Patch coverage is 79.24528% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 85.64%. Comparing base (6250307) to head (8922b5d).
Report is 210 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5527      +/-   ##
==========================================
+ Coverage   83.38%   85.64%   +2.26%     
==========================================
  Files         297      265      -32     
  Lines       12531    11315    -1216     
==========================================
- Hits        10449     9691     -758     
+ Misses       2082     1624     -458

Flag	Coverage Δ
unittests	`?`
unittests-Solution-Experimental	`85.60% <79.24%> (?)`
unittests-Solution-Stable	`85.37% <79.24%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...etryProtocol/Implementation/ExperimentalOptions.cs	`89.47% <81.81%> (-10.53%)`	⬇️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs	`89.93% <78.57%> (-3.69%)`	⬇️

... and 108 files with indirect coverage changes

github-actions · 2024-04-18T03:16:56Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2024-04-26T03:17:10Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

…b.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-opt-in-persistentstorage

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md

rajkumar-rangaraj · 2024-05-07T17:55:15Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ExperimentalOptions.cs

@@ -17,6 +17,8 @@ internal sealed class ExperimentalOptions

    public const string OtlpRetryEnvVar = "OTEL_DOTNET_EXPERIMENTAL_OTLP_RETRY";

+    public const string OtlpDiskRetryDirectoryPathEnvVar = "OTEL_DOTNET_EXPERIMENTAL_OTLP_DISK_RETRY_DIRECTORY_PATH";


If an app is designed to run in a platform-independent environment and wants to use a temporary location, the restriction to set the environment variable OTEL_DOTNET_EXPERIMENTAL_OTLP_DISK_RETRY_DIRECTORY_PATH can make things complex. Should we write to the temporary path if this environment variable is not set? We could rely on .NET API to get temp path Path.GetTempPath. Customers will use OTEL_DOTNET_EXPERIMENTAL_OTLP_RETRY to enable offline storage anyway. We could explain the behavior in the documentation along with this environment variable.

Works for me. Its more simple for users who want minimal configuration for trying out this feature.

CodeBlanch · 2024-05-07T17:56:08Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md

@@ -632,6 +632,11 @@ want to solicit feedback from the community.

    Added in `1.8.0`.

+    When set to `disk` along with setting


As a user, I think I would have a lot of questions about this feature. What are the file names? What is the file structure? What is the retention policy? Are there multiple files? What do these files mean? How are they managed? How are they cleaned up? Stuff like that. I think we may need more docs. Doesn't have to be on this PR.

+1, in addition, who has access to these files, any security/privacy concern, etc.

Agree we need to explain these.

I think it would be better to improve the doc here to cover these details: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.PersistentStorage.FileSystem. We could then link from here.

Code for persistent storage is vendored in this exporter, and it does not use a persistent storage package reference. It’s appropriate to document this here.

We have a process defined: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/PersistentStorage/README.md#persistent-storage-apis-for-otlp-exporter. Currently we are not forking Readmes, but I think we should do that since we do not make any customizations to the forked files.

CodeBlanch · 2024-05-07T17:56:50Z

Do we need to add unit tests?

vishweshbankwar · 2024-05-07T18:52:14Z

Do we need to add unit tests?

@CodeBlanch - Updated the tests. Some tests were also added previously in #5460

…b.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-opt-in-persistentstorage

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptionsExtensions.cs

CodeBlanch

LGTM

…b.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-opt-in-persistentstorage

rajkumar-rangaraj

LGTM

Add disk retry enablement

1f5a7a8

vishweshbankwar and others added 2 commits April 9, 2024 17:41

Merge branch 'main' into vibankwa/add-opt-in-persistentstorage

8981aca

rename env var

82464dd

github-actions bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Apr 18, 2024

github-actions bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Apr 26, 2024

vishweshbankwar and others added 7 commits May 1, 2024 11:12

Merge branch 'main' into vibankwa/add-opt-in-persistentstorage

526e290

refactory

97eaacc

Merge branch 'main' into vibankwa/add-opt-in-persistentstorage

0e6734c

changelog + readme

ee854f3

Merge branch 'vibankwa/add-opt-in-persistentstorage' of https://githu…

847d228

…b.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-opt-in-persistentstorage

update test

3417006

revert test

edf649e

vishweshbankwar marked this pull request as ready for review May 3, 2024 17:11

vishweshbankwar requested a review from a team May 3, 2024 17:11

CodeBlanch reviewed May 7, 2024

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md Outdated Show resolved Hide resolved

rajkumar-rangaraj reviewed May 7, 2024

View reviewed changes

CodeBlanch reviewed May 7, 2024

View reviewed changes

CodeBlanch added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label May 7, 2024

vishweshbankwar and others added 3 commits May 7, 2024 11:39

review

1371484

review

634a2be

Merge branch 'main' into vibankwa/add-opt-in-persistentstorage

d41f14d

vishweshbankwar added 2 commits May 7, 2024 12:03

changelog

d4eab4d

Merge branch 'vibankwa/add-opt-in-persistentstorage' of https://githu…

44b7bf0

…b.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-opt-in-persistentstorage

vishweshbankwar and others added 2 commits May 7, 2024 12:05

clean up

053d1bb

Merge branch 'main' into vibankwa/add-opt-in-persistentstorage

2a42d1a

CodeBlanch reviewed May 7, 2024

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptionsExtensions.cs Show resolved Hide resolved

CodeBlanch approved these changes May 7, 2024

View reviewed changes

vishweshbankwar added 2 commits May 7, 2024 13:00

address feedback

a909b96

Merge branch 'vibankwa/add-opt-in-persistentstorage' of https://githu…

2d7ea62

…b.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/add-opt-in-persistentstorage

rajkumar-rangaraj approved these changes May 7, 2024

View reviewed changes

add debug.assert

8922b5d

CodeBlanch merged commit 4215968 into open-telemetry:main May 7, 2024
35 checks passed

vishweshbankwar mentioned this pull request May 17, 2024

[OTLP] Add details for offline storage #5628

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Otlp] Add disk retry enablement #5527

[Otlp] Add disk retry enablement #5527

vishweshbankwar commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

github-actions bot commented Apr 18, 2024

github-actions bot commented Apr 26, 2024

rajkumar-rangaraj May 7, 2024

vishweshbankwar May 7, 2024

vishweshbankwar May 7, 2024

CodeBlanch May 7, 2024

reyang May 7, 2024

vishweshbankwar May 7, 2024 •

edited

Loading

rajkumar-rangaraj May 7, 2024

vishweshbankwar May 7, 2024

CodeBlanch commented May 7, 2024

vishweshbankwar commented May 7, 2024

CodeBlanch left a comment

rajkumar-rangaraj left a comment

		@@ -17,6 +17,8 @@ internal sealed class ExperimentalOptions

		public const string OtlpRetryEnvVar = "OTEL_DOTNET_EXPERIMENTAL_OTLP_RETRY";

		public const string OtlpDiskRetryDirectoryPathEnvVar = "OTEL_DOTNET_EXPERIMENTAL_OTLP_DISK_RETRY_DIRECTORY_PATH";

		@@ -632,6 +632,11 @@ want to solicit feedback from the community.

		Added in `1.8.0`.

		When set to `disk` along with setting

[Otlp] Add disk retry enablement #5527

[Otlp] Add disk retry enablement #5527

Conversation

vishweshbankwar commented Apr 9, 2024 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Apr 9, 2024 • edited Loading

Codecov Report

github-actions bot commented Apr 18, 2024

github-actions bot commented Apr 26, 2024

rajkumar-rangaraj May 7, 2024

Choose a reason for hiding this comment

vishweshbankwar May 7, 2024

Choose a reason for hiding this comment

vishweshbankwar May 7, 2024

Choose a reason for hiding this comment

CodeBlanch May 7, 2024

Choose a reason for hiding this comment

reyang May 7, 2024

Choose a reason for hiding this comment

vishweshbankwar May 7, 2024 • edited Loading

Choose a reason for hiding this comment

rajkumar-rangaraj May 7, 2024

Choose a reason for hiding this comment

vishweshbankwar May 7, 2024

Choose a reason for hiding this comment

CodeBlanch commented May 7, 2024

vishweshbankwar commented May 7, 2024

CodeBlanch left a comment

Choose a reason for hiding this comment

rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

vishweshbankwar commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

vishweshbankwar May 7, 2024 •

edited

Loading